Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - sql: add Database.WithConnection #6445

Closed
wants to merge 4 commits into from

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Nov 11, 2024

Motivation

In some situations (most notably, syncv2) a lot of database queries
need to be made with no need for a read or write transaction.
Database connection pool adds noticeable delays to the queries.
Using read transactions instead of simple connections has the side
effect of blocking WAL checkpoints.

Description

This adds a possibility to take a connection from the pool to use it
via the Executor interface, and return it later when it's no longer
needed. This avoids connection pool overhead in cases when a lot of
quries need to be made, but the use of read transactions is not
needed.

Additionally, this PR removes unused Rollback method from migrations.
The failed migrations are reverted either by rolling back the transactions
or dropping the temporary database used when vacuuming is needed.

UPD: Rollback method can be useful after all, so reverted this part of the PR.

This adds a possibility to take a connection from the pool to use it
via the Executor interface, and return it later when it's no longer
needed. This avoids connection pool overhead in cases when a lot of
quries need to be made, but the use of read transactions is not
needed.

Using read transactions instead of simple connections has the side
effect of blocking WAL checkpoints.
sql/database.go Outdated Show resolved Hide resolved
sql/database_test.go Outdated Show resolved Hide resolved
sql/interface.go Show resolved Hide resolved
sql/migrations.go Outdated Show resolved Hide resolved
sql/schema.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 54.83871% with 14 lines in your changes missing coverage. Please review.

Project coverage is 79.9%. Comparing base (f711c61) to head (571a53e).
Report is 15 commits behind head on develop.

Files with missing lines Patch % Lines
sql/database.go 44.0% 10 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           develop   #6445     +/-   ##
=========================================
- Coverage     79.9%   79.9%   -0.1%     
=========================================
  Files          352     352             
  Lines        46096   46126     +30     
=========================================
+ Hits         36865   36870      +5     
- Misses        7142    7161     +19     
- Partials      2089    2095      +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@ivan4th ivan4th changed the title sql: add Database.Connection/WithConnection, interface cleanup sql: add Database.WithConnection Nov 13, 2024
@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 13, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Nov 13, 2024
-------

## Motivation

In some situations (most notably, syncv2) a lot of database queries
need to be made with no need for a read or write transaction.
Database connection pool adds noticeable delays to the queries.
Using read transactions instead of simple connections has the side
effect of blocking WAL checkpoints.
@spacemesh-bors
Copy link

Build failed:

@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 14, 2024

systest failure:

=== FAIL: systest TestPartition_50_50 (133.53s)
    logger.go:146: 2024-11-13T21:32:41.022Z	INFO	TestPartition_50_50	testcontext/context.go:391	using	***"namespace": "test-iyhh"***
    logger.go:146: 2024-11-13T21:32:41.036Z	INFO	TestPartition_50_50	cluster/cluster.go:166	Using the following nodes	***"total": 20, "bootnodes": 2, "smeshers": 18, "old smeshers": 0, "remote": 0***
    partition_test.go:199: 
        	Error Trace:	/src/systest/tests/partition_test.go:199
        	Error:      	Received unexpected error:
        	            	watcher is terminated while waiting for pod with id smesher-0
        	Test:       	TestPartition_50_50

This looks like stuck network or smth like that.
Given that the new temporary connection mechanism isn't actually used anywhere yet in this PR, this is unrelated.

@fasmat
Copy link
Member

fasmat commented Nov 14, 2024

I'm not exactly sure what went wrong, but all smeshers were deployed quite late: https://grafana.spacemesh.dev/goto/wJ_klsGHg?orgId=1

smesher-0 came online ~22:34:40 and 1 second later the test runner decided to not try to ping it again and end the test. Normally the test should wait 5 (bootstrap-duration) minutes before giving up, I'm not sure why it either didn't came online within that time or why the runner gave up earlier 🤷

@fasmat
Copy link
Member

fasmat commented Nov 15, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Nov 15, 2024
## Motivation

In some situations (most notably, syncv2) a lot of database queries
need to be made with no need for a read or write transaction.
Database connection pool adds noticeable delays to the queries.
Using read transactions instead of simple connections has the side
effect of blocking WAL checkpoints.
@spacemesh-bors
Copy link

Build failed:

  • systest-status

@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 17, 2024

bors merge

spacemesh-bors bot pushed a commit that referenced this pull request Nov 17, 2024
## Motivation

In some situations (most notably, syncv2) a lot of database queries
need to be made with no need for a read or write transaction.
Database connection pool adds noticeable delays to the queries.
Using read transactions instead of simple connections has the side
effect of blocking WAL checkpoints.
@spacemesh-bors
Copy link

Pull request successfully merged into develop.

Build succeeded:

@spacemesh-bors spacemesh-bors bot changed the title sql: add Database.WithConnection [Merged by Bors] - sql: add Database.WithConnection Nov 17, 2024
@spacemesh-bors spacemesh-bors bot closed this Nov 17, 2024
@spacemesh-bors spacemesh-bors bot deleted the feature/long-db-conns branch November 17, 2024 21:46
@ivan4th ivan4th mentioned this pull request Nov 25, 2024
35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants